Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v2: Propagate panics, add AsyncHandle #1

Merged
merged 39 commits into from
Apr 4, 2021
Merged

v2: Propagate panics, add AsyncHandle #1

merged 39 commits into from
Apr 4, 2021

Conversation

andybarron
Copy link
Owner

@andybarron andybarron commented Apr 2, 2021

Code changes

  • Use std::panic to propagate panics from the thread pool into the async context, rather than triggering the Rayon panic handler.
  • Add AsyncRayonHandle type that implements Future, which makes the async-trait crate unnecessary.
  • Bypass Tokio RecvError. We control the Sender, so it should never be dropped too early.
  • Remove prelude module.
  • Seal AsyncThreadPool trait.

Repository changes

  • Update CI settings to support Rust 1.45.0 and up.
  • Update Codecov settings.
  • Add changelog.
  • Update README.

@codecov
Copy link

codecov bot commented Apr 2, 2021

Codecov Report

Merging #1 (a41ac04) into main (56c5259) will decrease coverage by 4.54%.
The diff coverage is 92.85%.

Impacted file tree graph

@@             Coverage Diff             @@
##              main       #1      +/-   ##
===========================================
- Coverage   100.00%   95.45%   -4.55%     
===========================================
  Files            2        3       +1     
  Lines           20       22       +2     
===========================================
+ Hits            20       21       +1     
- Misses           0        1       +1     
Impacted Files Coverage Δ
src/async_handle.rs 83.33% <83.33%> (ø)
src/async_thread_pool.rs 100.00% <100.00%> (ø)
src/global.rs 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 56c5259...a41ac04. Read the comment docs.

@coolreader18
Copy link

coolreader18 commented Apr 3, 2021

Coming from the reddit thread - I don't think you need to wrap tokio's RecvError, since it's only a unit error. There's no information in it, so you might as well a) have a unit error type of your own like SpawnFailed or b) just unwrap it, since if you're catching panics it shouldn't ever fail to send something through the channel.

@andybarron
Copy link
Owner Author

Coming from the reddit thread - I don't think you need to wrap tokio's RecvError, since it's only a unit error. There's no information in it, so you might as well a) have a unit error type of your own like SpawnFailed or b) just unwrap it, since if you're catching panics it shouldn't ever fail to send something through the channel.

That's a great point! It could only fail if the sender is dropped, but I'm controlling the sender, so that shouldn't ever happen. I'll just have it return unconditionally then :) Thanks!

src/async_handle.rs Outdated Show resolved Hide resolved
@andybarron andybarron changed the title v2: Propagate panics, add AsyncHandle and Error v2: Propagate panics, add AsyncHandle Apr 3, 2021
Copy link

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't have any panic tests that actually call the spawn method. They all just construct the handle directly.

src/async_handle.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/global.rs Outdated Show resolved Hide resolved
src/global.rs Show resolved Hide resolved
src/async_handle.rs Outdated Show resolved Hide resolved
@andybarron
Copy link
Owner Author

You don't have any panic tests that actually call the spawn method. They all just construct the handle directly.

Fixed!

@andybarron
Copy link
Owner Author

andybarron commented Apr 4, 2021

@Darksonn @coolreader18 Thanks so much for the feedback! Super helpful :) I'm going to go ahead and merge & release. If there's anything else you notice, feel free to file an issue, and I'll take a look at it 👍

@andybarron andybarron merged commit 420a73b into main Apr 4, 2021
@andybarron andybarron deleted the v2 branch April 4, 2021 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants